Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

refactor($compile): Create non-descriptive comments when debugInfoEnabled is false #14132

Closed
wants to merge 1 commit into from

Conversation

lgalfaso
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Refactor on generated comments used by $compile
  • What is the current behavior? (You can also link to an open issue here)
    Comments contain the directive name and expression as part of their data.
  • What is the new behavior (if this is a feature change)?
    When debugInfoEnabled is false, the comments will have empty data. When debugInfoEnabled
    is true, the current behavior is kept.
  • Does this PR introduce a breaking change?
    No
  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Other information:

When debugInfoEnabled is false when comments generated by transclusions, ngIf,
ngRepeat and ngSwitch will not contain any information about the directive nor
the expression associated with it.

Closes: #8722

@gkalpak
Copy link
Member

gkalpak commented Feb 25, 2016

ngMessagesInclude creates a similar comment. Maybe we could use $createComment() there too.

Other than that it LGTM if you can make JSCS happy 😉

@@ -1508,6 +1508,14 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
safeAddClass($element, isolated ? 'ng-isolate-scope' : 'ng-scope');
} : noop;

compile.$createComment = function(directiveName, comment) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be $$createComment (double $) like the other methods above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is $$createComment then for sure we should not use it in ngMessagesInclude

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$$ is a good idea :)
Why not use it is ngMessagesInclude then ? We use lots of $$-private stuff in external modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm... ok, updated the PR

@lgalfaso
Copy link
Contributor Author

ngMessagesInclude creates a similar comment. Maybe we could use $createComment() there too.

thought about it, but that would imply that it would need to check that this function is present... maybe not that bad

@gkalpak
Copy link
Member

gkalpak commented Feb 25, 2016

External modules (such as ngMessages) are only guaranted to work if used with the same version of core. There are several things that break (in several modules) if used with the incorrect version.

So, angular-messges.js v1.5.1 should only be used with angular.js v1.5.1. So, there shouldn't be an issue.

…bled is false

When debugInfoEnabled is `false` when comments generated by transclusions, ngIf,
ngRepeat and ngSwitch will not contain any information about the directive nor
the expression associated with it.

Closes: angular#8722
@lgalfaso
Copy link
Contributor Author

External modules (such as ngMessages) are only warranted to work if used with the same version of core.

but people are not that careful, and breaking things is always bad

@Narretz
Copy link
Contributor

Narretz commented Feb 25, 2016

Letting ngMessages use $createComent is a good idea. We've made a similar change in 1.4.9 where some function where moved from ngAnimate to ng, which made both incompatible with other 1.4.x versions. This is expected and we should take advantage of it here.

@lgalfaso
Copy link
Contributor Author

landed with 32feb2b
backported to 1.5.x with afeec23

@lgalfaso lgalfaso closed this Feb 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

how i can remove the comments created by ng-if and ng-repeat?
5 participants